-
Notifications
You must be signed in to change notification settings - Fork 214
feat: add ReturnType parser #2317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Looks good overall but the code feels more complex than it ought to be.
} else { | ||
// Case: ReturnType<SomeType["methodName"]> or other complex types | ||
// Get the type directly from TypeScript's type system | ||
const argType = this.checker.getTypeAtLocation(typeArg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really want to a lid the type checker if possible. What cases fail when we remove this branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When removing the else branch completely:
test/valid-data/type-return-type-complex/main.ts(15,28): error TSJ - 100: Unknown node of kind "TypeReference"
Error: Unknown node of kind "TypeReference"
at ReturnTypeNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/NodeParser/ReturnTypeNodeParser.ts:98:19)
at ChainNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/ChainNodeParser.ts:37:49)
at TypeAliasNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/NodeParser/TypeAliasNodeParser.ts:40:43)
at AnnotatedNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/NodeParser/AnnotatedNodeParser.ts:34:47)
at ExposeNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/ExposeNodeParser.ts:23:45)
at CircularReferenceNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/CircularReferenceNodeParser.ts:24:43)
at ChainNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/ChainNodeParser.ts:37:49)
at TopRefNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/TopRefNodeParser.ts:14:47)
at <anonymous> (/home/rockerboo/code/others/ts-json-schema-generator/src/SchemaGenerator.ts:31:39)
at Array.map (<anonymous>)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but what line of the example causes this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test/valid-data/type-return-type-complex/main.ts(15,28)
Specifically the node ReturnType
If you give me some more context of what to look at, I can take a deeper look to finding a better solution.
|
||
throw new UnknownNodeError(node); | ||
} catch (error) { | ||
throw error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a noop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the try/catch
} | ||
|
||
// Fallback to type checking method | ||
const type = this.checker.getTypeOfSymbolAtLocation(symbol, typeArg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as below. In what cases do we need the fallback? Why can't we use the function parser instead of implementing a lot of custom logic here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this fallback causes issues with implicit function return types. I added a test for this case.
Error: /home/rockerboo/code/others/ts-json-schema-generator/test-no-explicit-return.ts(5,34): error TSJ - 100: Unknown node of kind "TypeReference"
Error: Unknown node of kind "TypeReference"
at ReturnTypeNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/NodeParser/ReturnTypeNodeParser.ts:110:15)
at ChainNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/ChainNodeParser.ts:37:49)
at TypeAliasNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/NodeParser/TypeAliasNodeParser.ts:40:43)
at AnnotatedNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/NodeParser/AnnotatedNodeParser.ts:34:47)
at ExposeNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/ExposeNodeParser.ts:23:45)
at CircularReferenceNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/CircularReferenceNodeParser.ts:24:43)
at ChainNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/ChainNodeParser.ts:37:49)
at TopRefNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/TopRefNodeParser.ts:14:47)
at <anonymous> (/home/rockerboo/code/others/ts-json-schema-generator/src/SchemaGenerator.ts:31:39)
at Array.map (<anonymous>)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reuse some of the existing function parsing logic here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to poke at it in terms of utilizing the FunctionNodeParser but since it's like an abstraction on top of functions I couldn't find a simple way of doing that. I am not fully aware of how this all works, so I do not know how to make it work well enough like this. Also we have it separated as "functions" but this is more of a data structure format (object, number, boolean, string) because of the ReturnType unless it returns a function (which might need to be further tested).
I can try to make it better but I am just not sure how to connect them.
I added more tests for the different function types though, and also tested methods.
I went a little further into return type of functions returning functions, but got more into a gray area and make this PR more complex. But it wasn't too bad to implement. const x = () => () => 1;
type X = ReturnType<typeof x>; Generics with return type of functions returning functions or methods returning functions was a a bit more complex. const x = <T extends string>() => <T>(): T => "hello";
type X = ReturnType<typeof x>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arthurfiorette could you help review this?
try { | ||
const typeName = ts.isIdentifier(node.typeName) ? node.typeName.text : node.typeName.getText(); | ||
return typeName === "ReturnType" && node.typeArguments?.length === 1; | ||
} catch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a try catch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the try/catch.
return false; | ||
} | ||
|
||
// Check if it's a ReturnType reference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove unnecessary comments that just explain what happens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
Handles
ReturnType<typeof functionName>
ReturnType<SomeType['method']>
This was to support more complex types from Redux toolkit store.
This was written mostly by AI which I guided along to get it to know how to get the return type. So some issues might be in how it is being parsed. I can look through the feedback to improve it.
Context:
https://github.com/microsoft/TypeScript/blob/f14b5c8a2f0be503ac455054a91573c63f0e5088/src/services/services.ts#L974-L976
https://github.com/microsoft/TypeScript/blob/f14b5c8a2f0be503ac455054a91573c63f0e5088/src/services/codefixes/fixAwaitInSyncFunction.ts#L56-L67
Possibly related issues:
#1155
#1847
#1887
#1910